Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

upgraded the build/CI to use net8.0 #4876

Merged
merged 25 commits into from
Oct 2, 2023
Merged

upgraded the build/CI to use net8.0 #4876

merged 25 commits into from
Oct 2, 2023

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented Sep 22, 2023 •
edited
Loading

Towards #4437
Related to #4868

Changes

Upgraded the build/CI to use the .NET 8 SDK.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@codecov
Copy link

codecov bot commented Sep 22, 2023 •
edited
Loading

Codecov Report

Merging #4876 (7db50c3) into main (b22c7a9) will decrease coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4876      +/-   ##
==========================================
- Coverage   82.95%   82.94%   -0.02%     
==========================================
  Files         294      294              
  Lines       12206    12206              
==========================================
- Hits        10126    10124       -2     
- Misses       2080     2082       +2
Flag Coverage Δ
unittests 82.94% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 7 files with indirect coverage changes

@reyang reyang mentioned this pull request Sep 29, 2023
Merged
@Yun-Ting Yun-Ting changed the title add net8 ci upgraded the build/CI to use net8.0 Sep 29, 2023
@Yun-Ting Yun-Ting marked this pull request as ready for review September 29, 2023 21:46
@Yun-Ting Yun-Ting requested a review from a team September 29, 2023 21:46
@Yun-Ting Yun-Ting mentioned this pull request Sep 29, 2023
Closed
4 tasks
xiang17
xiang17 reviewed Sep 30, 2023
View reviewed changes
build/Common.props Outdated Show resolved Hide resolved
reyang
reyang approved these changes Sep 30, 2023
View reviewed changes
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to unblock net8 support, I've added few comments but these can be addressed in a follow up PR.

CodeBlanch
CodeBlanch reviewed Sep 30, 2023
View reviewed changes
test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs Outdated
@@ -912,6 +914,7 @@ void ConfigureTestServices(IServiceCollection services)
Assert.Equal(4, numberofSubscribedEvents);
}

#if !NET8_0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea what's going on here? Was something fixed or broken in .NET 8?

CodeBlanch
CodeBlanch reviewed Sep 30, 2023
View reviewed changes
test/OpenTelemetry.Instrumentation.AspNetCore.Tests/InProcServerTests.cs Outdated
@@ -14,7 +14,7 @@
// limitations under the License.
// </copyright>

#if NET6_0_OR_GREATER
#if NET6_0 || NET7_0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above. Any idea what happened here? Seems kind of odd something would change for .NET 8.

Copy link
Contributor Author

@Yun-Ting Yun-Ting Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodeBlanch:
I've updated the code to only run when !OSX || !NET8_0.
#4437 (comment)
I'm still trying to understand why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What error did you see if we don't make the change? @Yun-Ting

Copy link
Contributor Author

@Yun-Ting Yun-Ting Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some server error: https://pipelinesghubeus2.actions.githubusercontent.com/qUmkdRTOExOOUcipfw2r74VakBMGn99kKwI1fYYZyOCfUDlpFN/_apis/pipelines/1/runs/117515/signedlogcontent/4?urlExpires=2023-10-02T22%3A16%3A13.0407373Z&urlSigningMethod=HMACV1&urlSignature=ozmqVATdFNoLaQ7QAGD%2BGWhT9sAoEf55qGR5e%2FDrDBQ%3D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem to be flaky tests or transient error, which are not specific to TFMs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged this PR but please continue to look into this. I'm worried that we might have stumbled onto a .NET 8 bug and if so we should report it back to the team ASAP!

@CodeBlanch CodeBlanch merged commit 8ea6bb9 into open-telemetry:main Oct 2, 2023
@Yun-Ting Yun-Ting deleted the yunl/net8CI branch October 2, 2023 22:59
@reyang reyang mentioned this pull request Oct 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodeBlanch CodeBlanch CodeBlanch left review comments

@vishweshbankwar vishweshbankwar vishweshbankwar left review comments

@reyang reyang reyang approved these changes

@xiang17 xiang17 xiang17 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Yun-Ting @xiang17 @reyang @vishweshbankwar @CodeBlanch